Skip to content

Add DataSource/FileSource proto hooks and FileScanConfig serde - #23683

Open
kumarUjjawal wants to merge 5 commits into
apache:mainfrom
kumarUjjawal:issue-23497-datasource-proto-hooks
Open

Add DataSource/FileSource proto hooks and FileScanConfig serde#23683
kumarUjjawal wants to merge 5 commits into
apache:mainfrom
kumarUjjawal:issue-23497-datasource-proto-hooks

Conversation

@kumarUjjawal

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

File scan serialization currently depends on central type downcasts in datafusion-proto.

This PR adds the foundation needed to move each data source to its own protobuf hooks while preserving the existing wire format and fallback behavior.

It unblocks the concrete source migrations tracked by #23516, #23517, and #23518.

What changes are included in this PR?

  • Add feature-gated try_to_proto hooks to DataSource and FileSource.
  • Delegate serialization from DataSourceExec through FileScanConfig to its concrete FileSource.
  • Add shared FileScanConfig protobuf encoding and decoding in the datasource crate.
  • Keep the existing central serializer as the fallback for sources that still return Ok(None).
  • Preserve the legacy protobuf wire format.
  • Add compatibility test

Are these changes tested?

Yes

Are there any user-facing changes?

This adds non-breaking, feature-gated public serialization hooks for data source and file source implementations.

@github-actions github-actions Bot added proto Related to proto crate datasource Changes to the datasource crate labels Jul 18, 2026
@codecov-commenter

codecov-commenter commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.04523% with 127 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.82%. Comparing base (2f25454) to head (2e5364c).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/datasource/src/file_scan_config/mod.rs 47.82% 35 Missing and 1 partial ⚠️
...atafusion/datasource/src/file_scan_config/proto.rs 87.67% 6 Missing and 29 partials ⚠️
datafusion/proto/src/physical_plan/mod.rs 88.64% 19 Missing and 12 partials ⚠️
datafusion/datasource/src/proto.rs 89.06% 4 Missing and 10 partials ⚠️
datafusion/proto/src/physical_plan/from_proto.rs 50.00% 6 Missing ⚠️
datafusion/proto/src/physical_plan/to_proto.rs 54.54% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #23683      +/-   ##
==========================================
+ Coverage   80.75%   80.82%   +0.06%     
==========================================
  Files        1096     1098       +2     
  Lines      373588   374459     +871     
  Branches   373588   374459     +871     
==========================================
+ Hits       301687   302651     +964     
+ Misses      53898    53752     -146     
- Partials    18003    18056      +53     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@adriangb could you take a look whenever you get the time.

@adriangb

Copy link
Copy Markdown
Contributor

Instead of duplicating could we move the code as you've done but then rewrite serialize_file_scan_config/parse_protobuf_file_scan_config as delegating wrappers to the new inline methods? then we don't need to test byte equality, we avoid drift, etc.

Ok(FileGroup::new(files))
}

fn partitioned_file_to_proto(pf: &PartitionedFile) -> Result<protobuf::PartitionedFile> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this and partitioned_file_from_proto be visible same as #23496 so that it can be used in #23752 as well otherwise that PR duplicates same logic

);
}

mod file_scan_config_serde {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we un-indent this mod and add another #[cfg(test)] on top?

@buraksenn buraksenn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the work, I'll start working on sub issues today

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

LGTM. Thanks for the work, I'll start working on sub issues today

Thanks for the review.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@adriangb can you take a look.

@adriangb adriangb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you take a look at the comments about moving TryFromProto and the implementations for PartitionedFile and FileGroup? Ideally we could do that as precursor PRs.

Ok(FileGroup::new(files))
}

pub(crate) fn partitioned_file_to_proto(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this could be just fn (nothing outside of this module uses it)

})
}

pub(crate) fn partitioned_file_from_proto(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this could be just fn (nothing outside of this module uses it)

/// Each concrete [`FileSource::try_to_proto`]
/// wraps the returned value in its own `*ScanExecNode`. Byte-compatible with
/// the former `serialize_file_scan_config` in `datafusion-proto`.
pub fn to_proto_conf(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we stick to the naming we're using elsewhere try_to_proto, try_from_proto?

/// table schema via [`FileScanConfig::parse_table_schema_from_proto`]).
///
/// Byte-compatible with the former `parse_protobuf_file_scan_config`.
pub fn from_proto_conf(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same note about naming

FileScanConfig::parse_table_schema_from_proto(proto)
}

pub fn parse_protobuf_file_scan_config(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably will be deprecated in future PR?


pub(crate) fn partitioned_file_to_proto(
pf: &PartitionedFile,
) -> Result<protobuf::PartitionedFile> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a free-standing method, which duplicates:

impl TryFromProto<&PartitionedFile> for protobuf::PartitionedFile {
type Error = DataFusionError;
fn try_from_proto(pf: &PartitionedFile) -> Result<Self> {
let last_modified = pf.object_meta.last_modified;
let last_modified_ns = last_modified.timestamp_nanos_opt().ok_or_else(|| {
DataFusionError::Plan(format!(
"Invalid timestamp on PartitionedFile::ObjectMeta: {last_modified}"
))
})? as u64;
Ok(protobuf::PartitionedFile {
arrow_schema: pf
.arrow_schema
.as_ref()
.map(|s| s.as_ref().try_into())
.transpose()?,
path: pf.object_meta.location.as_ref().to_owned(),
size: pf.object_meta.size,
last_modified_ns,
partition_values: pf
.partition_values
.iter()
.map(|v| v.try_into())
.collect::<Result<Vec<_>, _>>()?,
range: pf
.range
.as_ref()
.map(protobuf::FileRange::try_from_proto)
.transpose()?,
statistics: pf.statistics.as_ref().map(|s| s.as_ref().into()),
})
}
}

I think if we move TryFromProto into datafusion-proto-models then we can depend on it here and just move the impl for these types and avoid duplicating it.

At the very least I think a better shape would be to

impl PartitionedFile {

  fn try_to_proto(...) { }
  fn try_from_proto(...) {}
}

adriangb added a commit to pydantic/datafusion that referenced this pull request Jul 30, 2026
…on-datasource (apache#24006)

## Which issue does this PR close?

- Part of apache#23494. Precursor for apache#23497 / apache#23683 (`DataSource` /
`FileSource` proto hooks) and for apache#23752.

## Rationale for this change

The protobuf conversions for the file-scan leaf types —
`PartitionedFile`,
`FileGroup`, `FileRange` — live in `datafusion-proto` as `TryFromProto`
impls,
because that is historically the only crate that can name both sides
(the
DataFusion type and the prost message are both foreign to it, hence the
`TryFromProto` workaround trait in the first place).

That placement means any *other* crate that needs those conversions has
to
reimplement them. apache#23683 hits exactly this: a `FileSource` serializing
its own
scan config needs to encode file groups, so the first cut of that PR
grew a
private second copy of the `PartitionedFile` wire logic inside
`datafusion-datasource`, which can then drift from the central
serializer.
The same will be true of every source migrated under apache#23516apache#23518.

Nothing about these conversions needs `datafusion-proto`: they are plain
data,
with `ScalarValue` / `Statistics` / `Schema` going through
`datafusion-proto-common`. They belong next to the types.

## What changes are included in this PR?

- New `datafusion_datasource::proto` module, behind a new `proto`
feature on
`datafusion-datasource` (off by default; `datafusion-proto` enables it):
  - `FileRange::try_to_proto` / `try_from_proto`
  - `PartitionedFile::try_to_proto` / `try_from_proto`
  - `FileGroup` <-> `protobuf::FileGroup`
- `datafusion-proto`'s `TryFromProto` impls for those types become
one-line
shims delegating to the new impls, so every existing caller keeps
working
  and the two sides cannot disagree.

### Why these are `TryFrom` and not `try_to_proto` hooks

`TryFromProto` exists because `datafusion-proto` owns neither side of
the
conversions it hosts: with both the DataFusion type and the prost
message
foreign to it, `impl TryFrom<protobuf::X> for X` is rejected by the
orphan
rule, so a local trait was the only way to say the same thing.

Moving a conversion into the crate that owns the DataFusion type removes
that
constraint, and `&T` is `#[fundamental]`, so both directions are
expressible
with the standard trait (checked, not assumed):

```rust
impl TryFrom<&protobuf::PartitionedFile> for PartitionedFile   // ok
impl TryFrom<&PartitionedFile> for protobuf::PartitionedFile   // ok
impl TryFrom<&[PartitionedFile]> for protobuf::FileGroup       // E0117
```

The last one is why `protobuf::FileGroup`'s *slice* conversion stays a
`TryFromProto` shim: `&[PartitionedFile]` is not a type this crate owns,
while
`&FileGroup` is. Callers inside DataFusion go through `FileGroup`.

So the rule this PR sets for the rest of apache#23494: **plain data uses
`TryFrom`;
anything needing an encode/decode context keeps the `try_to_proto(ctx)`
/
`try_from_proto(node, ctx)` hooks**, because the standard trait cannot
carry
that second argument. Usefully, none of the ~40
`TryFromProto`/`FromProto`
impls needs a context, and nothing that needs one was ever a
`TryFromProto`
impl — the two categories are already disjoint, so the shape now tells a
reader whether a conversion recurses.

### Why now

`FromProto` / `TryFromProto` were added in apache#21929, *after* the 54.0.0
release,
and 54.1.0 was cut before any of this landed — so they have never
shipped in a
release. Replacing them with the standard traits, and eventually
deleting them,
is a no-op for semver **today** and a major breaking change the moment
55.0.0
goes out. The same applies to the six inherent `try_to_proto` /
`try_from_proto`
methods this PR would otherwise have added: they are new, unreleased
API, so
choosing their final shape costs nothing right now.

The other reason to settle it here rather than in a follow-up: this is
the PR
that establishes the pattern for the data-source family (apache#23516-apache#23519
and
apache#23752 / apache#23781 are all queued behind it). Whichever shape merges first
is the
one they will copy.

Retiring the remaining ~34 impls is still its own follow-up. Two notes
for
whoever picks it up: the sink and format-option conversions can move
next to
their types the same way, but the ones for `datafusion-common`-owned
types
(`JoinType`, `NullEquality`, `TableReference`, `UnnestOptions`, ...)
cannot —
`datafusion-common` cannot depend on `datafusion-proto-models` (it is
underneath it via `datafusion-proto-common`). Their legal home is
`proto-models` itself, implementing on the local proto type, which is
already
how `proto-common` hosts the `ScalarValue` / `Statistics` conversions.

## Are these changes tested?

Yes.

- New unit tests in `datafusion_datasource::proto` covering the
`PartitionedFile` round trip (path, size, mtime, partition values,
range,
arrow schema, statistics), the `FileGroup` round trip, and the
invalid-path
  error.
- The existing `datafusion-proto` tests now exercise the delegating
shims, so
  they also pin the shims themselves.
- `datafusion-proto`, all features: 227 passed / 0 failed.
- `datafusion-datasource` with `proto`: 180 passed / 0 failed.
- Full workspace run: 10347 passed
(`cargo test --profile ci --workspace --lib --tests --features
avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`).
The only failures are 8 backtrace-symbolization tests in
`datafusion-common`,
a crate this PR does not touch and which sits below every crate it does;
they
  fail the same way on the base commit on macOS.
- `cargo fmt` and `ci/scripts/rust_clippy.sh` clean.

## Are there any user-facing changes?

The protobuf wire format is unchanged, and no existing API changes
shape.

Additive:

- New `proto` feature on `datafusion-datasource` (off by default).
- New `TryFrom` impls in both directions between `FileRange`,
  `PartitionedFile`, `FileGroup` and their protobuf messages, under that
feature. No new names are added to the crate's API surface: the trait is
  `core::convert::TryFrom`.

Note for reviewers: while writing the round-trip test I found that
`PartitionedFile` statistics do not round-trip cleanly on `main` — filed
as
apache#23998. This PR preserves that behavior exactly rather than changing
decode
semantics in a refactor; the test documents it.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

datasource Changes to the datasource crate proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proto: add DataSource/FileSource try_to_proto hook + port FileScanConfig serde

4 participants